Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add job_name to job_kwargs #1986

Closed
wants to merge 10 commits into from
Closed

Add job_name to job_kwargs #1986

wants to merge 10 commits into from

Conversation

h-mayorquin
Copy link
Collaborator

@h-mayorquin h-mayorquin commented Sep 13, 2023 •
edited
Loading

job_kwargs if the keyword arguments of ChunkRecordingExecutor but for some reason job_name was missing. This PR fixes that.

Based on this discussion #1799 (comment)

@h-mayorquin h-mayorquin added the refactor Refactor of code, with no change to functionality label Sep 13, 2023
@h-mayorquin h-mayorquin self-assigned this Sep 13, 2023
@h-mayorquin h-mayorquin mentioned this pull request Sep 13, 2023
Merged
@h-mayorquin h-mayorquin marked this pull request as ready for review September 13, 2023 11:21
samuelgarcia
samuelgarcia reviewed Sep 13, 2023
View reviewed changes
src/spikeinterface/core/node_pipeline.py Outdated
@@ -366,13 +365,14 @@ def run_node_pipeline(

init_args = (recording, nodes)

job_kwargs["job_name"] = "pipeline"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we want to modify in place a global job_kwargs dict we have in many script.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not modify the global job_kwargs here. It is just replicating the behavior that we had before.

Or maybe I am missing something?

@samuelgarcia
Copy link
Member

Finally I am not sure this is a good idea. (I change my mind often)

  1. job_kwargs is not only for ChunkrecordingExecutor but also more stuff.
  2. very often we
job_kwargs = dict(n_jobs=4, chunk_duration='2s')
my_parralel_func(..., **job_kwargs)

I do not want my job_kwargs to be modified my my_parralel_func() the job_name will be decided by my_parralel_func itself

@h-mayorquin
Copy link
Collaborator Author

h-mayorquin commented Sep 13, 2023 •
edited
Loading

Finally I am not sure this is a good idea. (I change my mind often)

  1. job_kwargs is not only for ChunkrecordingExecutor but also more stuff.

Can you point out this more stuff? This is important for me to know as I have been working with this assumption.

  1. very often we
job_kwargs = dict(n_jobs=4, chunk_duration='2s')
my_parralel_func(..., **job_kwargs)

I do not want my job_kwargs to be modified my my_parralel_func() the job_name will be decided by my_parralel_func itself

The name is still decided by the my_paralel_funct , the job_kwrgs is already being modified here:
https://github.com/h-mayorquin/spikeinterface/blob/87e5a4046f855d631da83e775000edad9c8f60e5/src/spikeinterface/core/job_tools.py#L53-L78

@alejoe91
Copy link
Member

We agreed that job_name should not be changed by the end user, but it is fixed by the functions that use the ChunkRecordingExecutor

@alejoe91 alejoe91 closed this Oct 25, 2023
@h-mayorquin h-mayorquin deleted the add_job_name_to_job_kwargs branch November 29, 2023 06:44
@h-mayorquin h-mayorquin mentioned this pull request Mar 20, 2024
Closed
@JoeZiminski JoeZiminski mentioned this pull request May 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuelgarcia samuelgarcia samuelgarcia left review comments

Assignees

@h-mayorquin h-mayorquin

Labels
refactor Refactor of code, with no change to functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@h-mayorquin @samuelgarcia @alejoe91